Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue with quotes around module params. #18

Merged
merged 1 commit into from
Nov 14, 2017
Merged

Fix issue with quotes around module params. #18

merged 1 commit into from
Nov 14, 2017

Conversation

josh-padnick
Copy link
Contributor

@josh-padnick josh-padnick commented Nov 14, 2017 •
edited
Loading

Previously, values passed to --module-param (e.g. --module-param "version=3.4.1") were rendering to with single quotes (e.g. '3.4.1'), whereas the single quotes should have been omitted (i.e. 3.4.1). This issue was causing some install.sh scripts that were receiving these module param values to fail.

This PR fixes the issue.

@josh-padnick
Fix issue with quotes around module params.
499e955 
Previously, a param was rendering as the literal " 'literal-val' " with single quotes, which was causing the script that was receiving this value to fail. Using double quotes removes the issue.
@josh-padnick
Copy link
Contributor Author

Feedback welcome as usual. Merging now.

@josh-padnick josh-padnick merged commit ecd67b9 into master Nov 14, 2017
@josh-padnick josh-padnick deleted the fix-quotes branch November 14, 2017 03:39
@brikis98
Copy link
Member

Not sure I follow what problem the single quotes were causing? Which install scripts were failing?

@josh-padnick
Copy link
Contributor Author

This is the type of error I was getting when passing in a --module-param:

ubuntu-ami: Downloading ZooKeeper '4.3.11'
ubuntu-ami: --2017-11-13 19:42:47--  http://www-us.apache.org/dist/zookeeper/zookeeper-'4.3.11'/zookeeper-'4.3.11'.tar.gz
ubuntu-ami: Resolving www-us.apache.org (www-us.apache.org)... 140.211.11.105
ubuntu-ami: Connecting to www-us.apache.org (www-us.apache.org)|140.211.11.105|:80... connected.
ubuntu-ami: HTTP request sent, awaiting response... 404 Not Found

@brikis98
Copy link
Member

That's most likely because you were on v0.0.16. In the very next version, I fixed the params by using eval: https://github.com/gruntwork-io/gruntwork-installer/releases/tag/v0.0.17. I suspect switching to double quotes as you did here had no effect; it was the upgrade past v0.0.16 that actually fixed it.

@brikis98
Copy link
Member

You and Matt may want to dig into the PRs I put up, even after they have been merged. It's important for all of us to keep up with these changes.

@josh-padnick
Copy link
Contributor Author

Hmm, we're duplicating each other's efforts on many fronts, it seems :/ Actually, I tested this code by copying the latest version in git to a Docker container and then running it there to repro the issue. It explicitly failed pre-double-quotes, and adding the double quotes explicitly fixed it.

Your general point on catching up on earlier PRs is well-taken, but for now, it seems like we're good here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@josh-padnick @brikis98